Conversation
|
Thanks for the review @ikruglov! |
| config: &crate::config::Config, | ||
| socket_path: &str, | ||
| ) -> std::result::Result<SystemdUnitStats, Box<dyn std::error::Error + Send + Sync>> { | ||
| ) -> anyhow::Result<SystemdUnitStats> { |
There was a problem hiding this comment.
Much nicer - I was going to look to clean these up / make thiserror custom errors ... might throw AI to double check later today
There was a problem hiding this comment.
Pull request overview
This PR addresses feedback from PR #158 by improving error handling and validation in the Varlink metrics collection code. The changes make the code more robust and maintainable by explicitly validating metric types before extraction and moving fallback logic to the appropriate architectural layer.
Changes:
- Enhanced error handling with explicit type validation and warning logs for invalid metrics
- Changed value extraction methods to panic on misuse, requiring proper validation before use
- Moved D-Bus fallback logic from
update_unit_statsto its callers for better separation of concerns - Updated metric names from snake_case to PascalCase to match systemd's metrics API format
- Simplified
VarlinkConfigto use derive Default
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/varlink/metrics.rs | Changed value extraction methods to require validation and panic on misuse instead of silently defaulting |
| src/varlink_units.rs | Added explicit type validation before metric extraction, updated metric names to PascalCase, removed D-Bus fallback |
| src/machines.rs | Added D-Bus fallback logic for container metrics collection with proper error handling |
| src/lib.rs | Added D-Bus fallback logic for host metrics collection with proper error handling |
| src/config.rs | Simplified VarlinkConfig to use derive Default |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "UnitLoadState" => { | ||
| if should_skip_unit(&object_name, config) { | ||
| return Ok(()); | ||
| } | ||
| let value = metric.value_as_string("" /* default_value */); | ||
| if !metric.value().is_string() { | ||
| warn!( | ||
| "Metric {} has non-string value: {:?}", | ||
| metric.name(), | ||
| metric.value() | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| let value = metric.value_as_string(); | ||
| let load_state = match SystemdUnitLoadState::from_str(value) { | ||
| Ok(v) => v, | ||
| Err(_) => { | ||
| warn!( | ||
| "Metric {} has unrecognized value: {:?}", | ||
| metric.name(), | ||
| value | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| }; | ||
| let unit_state = stats | ||
| .unit_states | ||
| .entry(object_name.to_string()) | ||
| .or_default(); | ||
| unit_state.load_state = | ||
| SystemdUnitLoadState::from_str(value).unwrap_or(SystemdUnitLoadState::unknown); | ||
| unit_state.load_state = load_state; | ||
| unit_state.unhealthy = | ||
| is_unit_unhealthy(unit_state.active_state, unit_state.load_state); | ||
| } |
There was a problem hiding this comment.
The UnitLoadState case duplicates validation and parsing logic that already exists in the parse_metric_enum helper function. The UnitActiveState case (lines 71-86) uses parse_metric_enum correctly, but this case reimplements the same logic inline. Consider refactoring to use parse_metric_enum for consistency and to reduce code duplication.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This addresses @ikruglov's comments on #158.